-
-
Notifications
You must be signed in to change notification settings - Fork 826
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(preset-icons): Add CSS SVG Sprite support #2675
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for unocss ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Issues:
Therefore, icons that contain |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Right now a few modules have been copied from iconify PR, later we can remove them:
|
# Conflicts: # examples/vite-vue3/src/App.vue # playground/src/auto-imports.d.ts
After testing sprites, I'm not sure adding support for CSS sprites is a good idea because of massive amount of bugs. TLDR CSS sprites are rendered correctly in all browsers only if:
Bugs
Short demo This is a screenshot of test page where I've been testing using various icons in CSS sprite: It contains screenshots of same page rendered in 3 browsers (from left to right): Safari, Chrome, Firefox. That page contains bunch of icons rendered as sprites (last 2 rows are tests for DOM sprites) Firefox (last part on the right) is the only browser that renders sprites correctly. Chrome and Safari have bugs and they are different. Scaling The biggest issue by far is scaling. I've made a bug report to Chromium about this: https://bugs.chromium.org/p/chromium/issues/detail?id=1471377 Users want to resize and position icons. That means aspect ratio must be maintained when icon is resized. Expecting users to calculate aspect ratio for each icon is unreasonable, so the only icons usable in sprites are square icons. While vast majority of icons in Iconify icon sets are square, there will always be edge cases and users will likely want to use it with custom icons too. Overflow This is related to scaling. If icon has different aspect ratio, Safari shows parts of nearby icons. However, this is not an issue because Chrome, which is the most used browser, doesn't even get to this point because it absolutely fails at scaling icon and ignores background-size. WebKit bug report: https://bugs.webkit.org/show_bug.cgi?id=259970 I suspect that when Chrome team fixes scaling bug, it will face the same overflow bug. Additionally, initial value for background-position is wrong in Safari (and possibly Chrome, but again scaling bug prevents from testing), which is another bug to consider. Conclusion I think SVG sprites in CSS is a bad idea. When using SVG sprites in DOM, it reduces duplication. But in CSS? I don't see any advantages, other than reducing number of requests to server, which can be solved by embedding icons in CSS using |
I only test with a few square icons, no idea how bad sprites work in browsers. It is about reducing css size, if you use a set of icons in your app having a Sprite or a set of sprites will reduce css with a few requests to the server (for example, the unocss-mdi icon set in my vuetify nuxt module). |
It doesn't reduce CSS size. Standard SVG: Entry for one icon in sprite: So entry for one icon in sprite is actually bigger than icon when its not in sprite. |
@cyberalien we're removing encoded svg from the url and moving the svg content to the Sprite, we're reducing a lot the css size. The full bundle will be bigger. |
While CSS size is smaller, sprite is loaded anyway, so it is just splitting content. Alternative solution to splitting content is to reference a second CSS, which assigns encoded SVG to variables, which are then used in main CSS:
then import that CSS file separately and use variables. Icons will be treated as separate SVGs, so all bugs I've mentioned above do not apply. |
After a lot of consideration, I think CSS sprites is not a good idea. Bugs mentioned above are big issues, the biggest one by far is aspect ratio in Chrome. Both Chrome and Safari bugs have been reported, confirmed, but it will take a while to get fixed. Until those bugs are fixed, CSS SVG sprites are not usable. Then, after bugs are fixed, it will take time for them to make it into latest versions of browsers. Then it will take even more time for users to update their browsers. While project is aimed at developers, who usually use the latest versions of browsers, actual websites that will display those icons are often aimed at regular folks, who are not so fast to update browsers. I think CSS SVG sprites will not be usable in next few years, therefore I'm against adding support for it. |
@cyberalien This is a bit unfortunate. I was looking forward to this feature and intended to use it with monochrome square icons (many popular icon libraries do meet the criteria for rendering correctly you listed above) Using a sprite reduces total css size, the sprite can also be loaded in parallel and changes less frequently which improves immutable cache hit rate when you deploy a new version. I hope this will be revisited some day, maybe with checks in place that just throw if safe conditions aren't met by an icon that goes to a sprite. |
You can achieve the same using separate CSS file with variables, as shown in example above, without triggering any bugs. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This PR includes:
sprite-
iconsvite-vue3
There is an SVG Sprite example in Vue 3 example to play with it, it is quite sensible, we cannot use same approach when adding SVG to the DOM via
<use href="..." />
.The PR is in draft, we need to finish build in Vite Plugin, add some tests and update docs: if looks good for you I'll try to finish it.
We need to fix:
@iconify/utils
repo and update the logic hereTODO list:
/cc @antfu